Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memoize _log.prefix calls #17355

Merged
merged 1 commit into from
Apr 27, 2018
Merged

Memoize _log.prefix calls #17355

merged 1 commit into from
Apr 27, 2018

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Apr 27, 2018 •
edited
Loading

In situations where an EMS has a large number of hosts, the tree that gets built is quite large, and so the number of times that _log.prefix is fired is very high. This updates each one of those calls to memoize them to a instance variable, since the resulting prefix will not change.

Benchmarks

The benchmark script used here basically runs the following:

ManageIQ::Providers::Vmware::InfraManager::ProvisionWorkflow.new.init_from_dialog

And is targeting a fairly large EMS, with about 600 hosts.

ms queries rows
before 15023 1961 48395
after 10056 1961 48395 
Before                                                     After
------                                                     -----

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 1034363078 bytes (19721183 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app                               |      5561668  manageiq/app
   3513258  manageiq/lib  <<<<<<<<<<                   |      2512007  activerecord-5.0.7
   2512007  activerecord-5.0.7                         |       861272  activesupport-5.0.7
    861270  activesupport-5.0.7                        |       418737  ancestry-2.2.2
    418737  ancestry-2.2.2                             |       278793  activemodel-5.0.7
    278793  activemodel-5.0.7                          |       178419  ruby-2.3.3/lib
    178419  ruby-2.3.3/lib                             |       165577  arel-7.1.4
    165577  arel-7.1.4                                 |        52875  manageiq-providers-vmware-0be2f13a0dc9
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        14424  fast_gettext-1.2.0
     14424  fast_gettext-1.2.0                         |         7903  manageiq/lib  <<<<<<<<<<
       ...                                             |          ...

Note: The benchmarks for this change do NOT include the changes from other PRs (#17354 for example). Benchmarks of all changes can be found here.

Links

@NickLaMuro
Memoize _log.prefix calls
c982f6c 
This is a relatively expensive method for how many times it is called,
and the cases in which is being used, it doesn't change it's value ever
between subsequent calls.

This simply memoizes the value that is generated, which very much
decreases the amount of duplicate work that is done for these methods.
@miq-bot
Copy link
Member

miq-bot commented Apr 27, 2018

Checked commit NickLaMuro@c982f6c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

This was referenced Apr 27, 2018
Merged
Merged
Merged
Closed
Merged
Merged
@NickLaMuro
Copy link
Member Author

@miq-bot assign @gmcculloug
@miq-bot add_label performance

@bdunne bdunne merged commit c2e01c9 into ManageIQ:master Apr 27, 2018
@bdunne bdunne assigned bdunne and unassigned gmcculloug Apr 27, 2018
@bdunne bdunne added this to the Sprint 85 Ending May 7, 2018 milestone Apr 27, 2018
This was referenced May 3, 2018
Merged
Merged
Closed
@NickLaMuro NickLaMuro mentioned this pull request May 14, 2018
Merged
@NickLaMuro NickLaMuro mentioned this pull request May 21, 2018
Closed
simaishi pushed a commit that referenced this pull request Jun 21, 2018
@bdunne @simaishi
Merge pull request #17355 from NickLaMuro/memoize_log_prefix_calls_in…
ef6b3f9 
…_miq_request_workflow

Memoize _log.prefix calls
(cherry picked from commit c2e01c9)

https://bugzilla.redhat.com/show_bug.cgi?id=1593798
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit ef6b3f9c75781c49d77b4d71b36d57c6ed8a8d6a
Author: Brandon Dunne <[email protected]>
Date:   Fri Apr 27 18:28:23 2018 -0400

    Merge pull request #17355 from NickLaMuro/memoize_log_prefix_calls_in_miq_request_workflow
    
    Memoize _log.prefix calls
    (cherry picked from commit c2e01c9bf8426239d1f691d93fa21ef78997304f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593798

simaishi pushed a commit that referenced this pull request Jun 21, 2018
@bdunne @simaishi
Merge pull request #17355 from NickLaMuro/memoize_log_prefix_calls_in…
a07dd77 
…_miq_request_workflow

Memoize _log.prefix calls
(cherry picked from commit c2e01c9)

https://bugzilla.redhat.com/show_bug.cgi?id=1593797
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit a07dd77014653443a748df1045d7030279b9ba4d
Author: Brandon Dunne <[email protected]>
Date:   Fri Apr 27 18:28:23 2018 -0400

    Merge pull request #17355 from NickLaMuro/memoize_log_prefix_calls_in_miq_request_workflow
    
    Memoize _log.prefix calls
    (cherry picked from commit c2e01c9bf8426239d1f691d93fa21ef78997304f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bdunne bdunne

Labels
bug fine/backported gaprindashvili/backported lifecycle/provisioning performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NickLaMuro @miq-bot @simaishi @bdunne @gmcculloug